-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(native): add a compressed minidump endpoint example #11304
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
I added @markushi and @JoshuaMoelans since the basic compressed example is relatively simple, but adding meta-data is quite involved. I would love to know if the steps and reasoning are understandable, provided the user is technically sophisticated enough to use |
Bundle ReportChanges will increase total bundle size by 154.8kB (1.1%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
# Add additional data via JSON or using the bracket syntax (the latter requiring a field for each entry) | ||
printf -- 'Content-Disposition: form-data; name="sentry"\r\n\r\n' >> multipart_body.txt | ||
printf -- '{"release":"[email protected]","tags":{"mytag":"value"}}\r\n' >> multipart_body.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first time going through these steps I didn't change this "release" to the actual version that the .dmp file was built for, which I think is the reason I got an {"detail":"invalid minidump"}%
result.
Not sure if the advanced users that would use this method of sending minidumps would make the same mistake, but still might be something worth highlighting.
Other than that, these instructions seem fine as a beginner's guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first time going through these steps I didn't change this "release" to the actual version that the .dmp file was built for, which I think is the reason I got an
{"detail":"invalid minidump"}%
result.
The main reason you could get an "invalid minidump"
(besides the obvious attaching an invalid minidump file 😅) is probably due to bad escaping or delimiting characters because those could prevent the receiving parser from "seeing" the minidump field.
Correctly formed meta-data shouldn't affect the validity of the minidump (i.e., Sentry doesn't cross-check the release
in the sentry
field with the contents of the minidump).
This is why documenting this as a solution is a double-edged sword. Manually constructing a multipart body can quickly introduce encoding errors like turns out I used ” instead of "
. Thankfully, that error appeared when parsing the JSON metadata (with a slightly better error message).
But if the encoding error appears beyond the field value boundary (forgetting the correct number of \r\n
and similar), you might get the results of a wrongly parsed HTTP request.
I think the best solution to the problem would be to let relay
- check if the file uploaded as
upload_file_minidump
is agzip
and in that case - unzip the file and then validate it as a minidump and continue processing as it does now
This would allow users to apply the same upload methods as described in the previous sections, but instead of uploading a mini.dmp
, they would upload a mini.dmp.gz
(identified via the first n-byte header).
Until that solution is implemented and deployed, users have a documented approach for uploading minidumps manually.
Following the conversation here: #11304 (comment) I created a Relay-PR (getsentry/relay#4029) that would reduce this documentation change to:
Let's wait before merging if the Relay folk consider this a sensible idea. |
I minimized the docs to the outcome of getsentry/relay#4029, which the relay team merged today. Let's wait until it is deployed so we can test it before we merge the changes to the docs. |
I tested this with all four supported compression containers against a public ingest endpoint, and it worked as expected. If you do not have any further adaptations (given the drastic content reduction compared to the initial proposal), @vivianyentran, and if the short version also makes sense to you, @JoshuaMoelans, I feel this is good to merge. |
lgtm! |
Co-authored-by: vivianyentran <[email protected]>
Co-authored-by: vivianyentran <[email protected]>
546e3c1
to
8bc3b3f
Compare
DESCRIBE YOUR PR
The docs currently do not describe how the minidump endpoint can be used when sending compressed payloads. This is often necessary when customers produce minidumps bigger than our upload limits.
Fixes #7575.
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes: